backends: name→package registry — decouple the name layer from the closed enum (#386 Phase 5) - #397
Conversation
#386 Phase 5) Introduce src/backend_registry.zig — a string-keyed registry that derives a backend's package facts (subpath backends/{name}, zon_name labelle_{name}, link_name labelle-{name}) from a plain name string instead of the closed config.Backend enum tag. This is the pluggability seam: lookup() resolves a name that has NO enum tag, so a future resolver can hand the name layer a third-party backend name. Add config.ProjectConfig.backendName() as the seam future code reads instead of @TagName(cfg.backend). The enum stays the backward-compat shorthand; parsing arbitrary names is the explicit follow-up. Route the ~8 name-derivation sites through the registry / backendName(): deps_linker (backend dep entry), build_files (build.zig.zon backend dep path), root.zig (backend template subpath + target dir name), main.zig (target dir name), gui_resolve.zig (3 diagnostics). Behavioral switch(cfg.backend) sites (codegen selection, gamepad sub-package staging) are intentionally left alone — those are the manifest splice's job, not the name layer. Tests: pluggability (fictional backend resolves), drift guard (builtin_names <-> Backend tags agree both ways), and a per-built-in inline-convention match. Byte-identical verified: bgfx-desktop + raylib-desktop build.zig / build.zig.zon / game.zig + target dir names unchanged before/after.
…hase-5 seam locator) The manifest-splice locator was the last @TagName(cfg.backend) in code (the registry PR's original base predated manifest_splice.zig, so the agent couldn't reach it). Now routed through backend_registry.lookup(cfg.backendName()) — the only residual enum coupling is config PARSING (.backend is still the enum).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a backend registry module for deriving backend package names and paths, introduces a ChangesBackend Registry Introduction and Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a centralized backend_registry module to decouple backend package-layout conventions from a closed enum, routing lookups through string names instead. The review feedback identifies critical memory leak vulnerabilities in src/backend_registry.zig and src/deps_linker.zig during allocation failures, recommending the use of errdefer for proper resource cleanup. Additionally, it suggests replacing std.debug.print with standard logging facilities (std.log.err) for user-facing errors in src/gui_resolve.zig to align with CLI best practices.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| pub fn lookup(allocator: std.mem.Allocator, name: []const u8) !BackendInfo { | ||
| return .{ | ||
| .name = name, | ||
| .subpath = try std.fmt.allocPrint(allocator, "backends/{s}", .{name}), | ||
| .zon_name = try std.fmt.allocPrint(allocator, "labelle_{s}", .{name}), | ||
| .link_name = try std.fmt.allocPrint(allocator, "labelle-{s}", .{name}), | ||
| }; | ||
| } |
There was a problem hiding this comment.
If any of the subsequent std.fmt.allocPrint calls fail (e.g., for zon_name or link_name), the previously allocated fields (like subpath) will be leaked. Use errdefer to clean up partially allocated fields on failure.
pub fn lookup(allocator: std.mem.Allocator, name: []const u8) !BackendInfo {
const subpath = try std.fmt.allocPrint(allocator, "backends/{s}", .{name});
errdefer allocator.free(subpath);
const zon_name = try std.fmt.allocPrint(allocator, "labelle_{s}", .{name});
errdefer allocator.free(zon_name);
const link_name = try std.fmt.allocPrint(allocator, "labelle-{s}", .{name});
return .{
.name = name,
.subpath = subpath,
.zon_name = zon_name,
.link_name = link_name,
};
}
| const backend_info = try backend_registry.lookup(allocator, cfg.backendName()); | ||
| defer allocator.free(backend_info.subpath); | ||
| const backend_path = try cache.resolveBundledPackage(allocator, cfg.labelle_version, cfg.assembler_version, project_dir, backend_info.subpath); | ||
| // zon_name / link_name are moved into the DepEntry (freed by | ||
| // freeDepEntries), so we don't free them here. | ||
| try deps.append(allocator, .{ .zon_name = backend_info.zon_name, .link_name = backend_info.link_name, .abs_path = backend_path }); |
There was a problem hiding this comment.
If cache.resolveBundledPackage or deps.append fails, backend_info.zon_name and backend_info.link_name will be leaked because they are not freed by defer allocator.free(backend_info.subpath). Additionally, backend_path will leak if deps.append fails. Use errdefer to ensure these resources are freed on failure.
const backend_info = try backend_registry.lookup(allocator, cfg.backendName());
defer allocator.free(backend_info.subpath);
errdefer allocator.free(backend_info.zon_name);
errdefer allocator.free(backend_info.link_name);
const backend_path = try cache.resolveBundledPackage(allocator, cfg.labelle_version, cfg.assembler_version, project_dir, backend_info.subpath);
errdefer allocator.free(backend_path);
// zon_name / link_name are moved into the DepEntry (freed by
// freeDepEntries), so we don't free them here.
try deps.append(allocator, .{ .zon_name = backend_info.zon_name, .link_name = backend_info.link_name, .abs_path = backend_path });
|
|
||
| const bridge_def = getBridgeForBackend(bridges, cfg.backend) orelse { | ||
| std.debug.print("labelle: GUI plugin '{s}' requires a bridge for backend '{s}', but none is declared in gui.labelle.\n", .{ manifest.name, @tagName(cfg.backend) }); | ||
| std.debug.print("labelle: GUI plugin '{s}' requires a bridge for backend '{s}', but none is declared in gui.labelle.\n", .{ manifest.name, cfg.backendName() }); |
There was a problem hiding this comment.
Avoid using std.debug.print for user-facing errors in CLI tools. Use standard logging facilities like std.log.err instead.
std.log.err("labelle: GUI plugin '{s}' requires a bridge for backend '{s}', but none is declared in gui.labelle.", .{ manifest.name, cfg.backendName() });
References
- In CLI tools, use standard logging facilities (e.g.,
std.log.warn) or write tostderrfor user-facing warnings, rather than using debug-specific print functions (e.g.,std.debug.print).
| bridge_dir = try std.fs.path.resolve(allocator, &.{ plugin_dir, rel_path }); | ||
| } else { | ||
| std.debug.print("labelle: GUI plugin '{s}' bridge for '{s}' has no .path (remote bridge resolution not yet supported)\n", .{ manifest.name, @tagName(cfg.backend) }); | ||
| std.debug.print("labelle: GUI plugin '{s}' bridge for '{s}' has no .path (remote bridge resolution not yet supported)\n", .{ manifest.name, cfg.backendName() }); |
There was a problem hiding this comment.
Avoid using std.debug.print for user-facing errors in CLI tools. Use standard logging facilities like std.log.err instead.
std.log.err("labelle: GUI plugin '{s}' bridge for '{s}' has no .path (remote bridge resolution not yet supported)", .{ manifest.name, cfg.backendName() });
References
- In CLI tools, use standard logging facilities (e.g.,
std.log.warn) or write tostderrfor user-facing warnings, rather than using debug-specific print functions (e.g.,std.debug.print).
|
|
||
| if (bridge_def.adapter.len == 0) { | ||
| std.debug.print("labelle: GUI plugin '{s}' bridge for '{s}' has empty .adapter name\n", .{ manifest.name, @tagName(cfg.backend) }); | ||
| std.debug.print("labelle: GUI plugin '{s}' bridge for '{s}' has empty .adapter name\n", .{ manifest.name, cfg.backendName() }); |
There was a problem hiding this comment.
Avoid using std.debug.print for user-facing errors in CLI tools. Use standard logging facilities like std.log.err instead.
std.log.err("labelle: GUI plugin '{s}' bridge for '{s}' has empty .adapter name", .{ manifest.name, cfg.backendName() });
References
- In CLI tools, use standard logging facilities (e.g.,
std.log.warn) or write tostderrfor user-facing warnings, rather than using debug-specific print functions (e.g.,std.debug.print).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/backend_registry.zig`:
- Around line 53-59: The BackendRegistry.lookup allocation path can leak memory
if a later allocPrint fails because earlier strings are never freed before the
function returns an error. Update lookup to build BackendInfo incrementally
using the same allocator, and add errdefer cleanup for each successfully
allocated field (subpath, zon_name, link_name) so partial allocations are
released on failure. Use the lookup function and the BackendInfo fields as the
places to apply the fix.
In `@src/deps_linker.zig`:
- Around line 55-60: The fallback path in `deps_linker.zig` is leaking
allocations if `cache.resolveBundledPackage` or `deps.append` fails. In the
`backend_registry.lookup` flow, add cleanup for `backend_info.zon_name`,
`backend_info.link_name`, and `backend_path` before the dep is appended, while
keeping the ownership transfer to `DepEntry` only on success; use the existing
`backend_info`/`deps.append` block to ensure all allocated values are freed on
error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 064958ab-d236-42a8-b12a-9a8f5cc44204
📒 Files selected for processing (8)
src/backend_registry.zigsrc/build_files.zigsrc/codegen/manifest_splice.zigsrc/config.zigsrc/deps_linker.zigsrc/gui_resolve.zigsrc/main.zigsrc/root.zig
- backend_registry.lookup: allocate subpath/zon_name/link_name incrementally with errdefer so a mid-sequence OOM doesn't leak the already-allocated fields (the struct never returns → caller never frees). - deps_linker: tight nested scope with errdefer for the moved-into-DepEntry fields (zon_name/link_name/backend_path) — frees them on any error up to the append, but NOT after (ownership → deps), and scoped so the later gamepad appends can't re-trigger a double-free. Both Gemini + CodeRabbit. (gui_resolve debug.print are pre-existing — the PR only swapped the name arg; left for a separate file-wide log cleanup.)
|
Addressed the two leak findings ( The 3 |
1 participant
The name→package registry — the Phase-5 pluggability seam. Centralizes every scattered
@tagName(cfg.backend)-based package-layout derivation into one string-keyed module, so the splice/codegen no longer reach the closedBackendenum for a backend's identity.What
src/backend_registry.zig—lookup(allocator, name) → BackendInfo { name, subpath "backends/{name}", zon_name "labelle_{name}", link_name "labelle-{name}" }, keyed by string (resolves names that are NOT enum tags — the point).builtin_namesis comptime-derived fromconfig.Backendso registry/enum can't drift. +isBuiltin,free.config.backendName()— the seam future code reads (returns@tagName(self.backend)for now;.backendparsing unchanged).deps_linker,build_files:745,root(target dir +loadBackendTemplate),main:217,gui_resolvediagnostics, and the manifest-splice'sbackendPackageDirlocator (the seam flagged in bgfx: manifest-driven codegen-splice for bgfx-desktop (epic #386 Phase 3/5) #396).switch(cfg.backend)sites (gamepad sub-packages, codegen fragments) — those select backend-specific behavior, the manifest-splice's job, not the name layer.Result
Zero
@tagName(cfg.backend)remain in code (only comments). The only thing still coupling a backend to the enum is config parsing (.backendis the closed enum) + the behavioral switches — exactly what the next step (open the resolver) tackles.Verified
build.zig/build.zig.zon/game.zig+ target dirs before/after (the registry just centralizes existing derivations).lookup("fictional")resolves with no enum tag (isBuiltinfalse) — the seam works.config.Backendtag ↔builtin_names.zig build testgreen (+4 registry tests). Rebased onto current main (includes the bgfx: assert the render contract — contract trifecta complete (#386 Phase 3) #395 render-assert + bgfx: manifest-driven codegen-splice for bgfx-desktop (epic #386 Phase 3/5) #396 manifest-splice); the manifest path now routes its package dir through the registry too.Part of epic #386 Phase 5. Next: open
configto parse an arbitrary backend name+package (the enum becomes a fast-path shorthand).Summary by CodeRabbit
New Features
Bug Fixes
Tests